Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pack of doc changes for SQL queries #4042

Merged
merged 1 commit into from
Sep 20, 2022
Merged

Pack of doc changes for SQL queries #4042

merged 1 commit into from
Sep 20, 2022

Conversation

roji
Copy link
Member

@roji roji commented Sep 19, 2022

  • Switch to showing the new FromSql by default.
  • Clearly document FromSqlRaw as appropriate only for dynamic SQL scenarios.
  • Possibly controversial: this renames the raw SQL "topic" to "SQL queries". The problem with "raw" is that its used in one of the APIs (FromSqlRaw) but not on the others (is FromSqlInterpolated a raw SQL query? :trollface:)
  • Documented the new SqlQuery API for primitive types
  • Added a "Non-querying SQL" section under the "SQL queries" page, for ExecuteSql. Yeah, it's weird - but doing a whole page just for that seemed really excessive.

Closes #3971
Closes #4041
Closes #1787

@roji roji changed the title Sql queries Pack of doc changes for SQL queries Sep 19, 2022
@roji roji requested a review from a team September 19, 2022 15:15
@roji roji marked this pull request as ready for review September 19, 2022 15:15

[!code-csharp[Main](../../../samples/core/Querying/SqlQueries/Program.cs#FromSqlAsNoTracking)]

## Querying primitive (non-entity) types
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


This code doesn't work, since database do not allow parameterizing column names (or any other part of the schema).

First, carefully consider whether you really want to fully parameterize the column name. For example, users may choose a column that isn't indexed, making the query run extremely slowly and overload your database. Except for truly dynamic scenarios, it's usually better to have two SQL queries for two column names, rather than using parameterization to collapse them into a single SQL query.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure this really belongs here; the same is true when using the more common LINQ method builder to choose a property dynamically. I.e. it's not a consequence of using raw SQL. But I don't feel strongly.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's true that you have the same consideration with EF.Property - assuming you pass it arbitrary input, and not a fixed string (e.g. because of a shadow property - which I think is what it's usually used for).

The idea here was to make the user fully understand what it is they're doing when they interpolate a fully dynamic column name into their SQL (and possibly dissuade them from it, unless it's really required)... I do agree that a similar note could be a good idea for EF.Property as well.

I still think it's not a bad fit here to help "overall understanding/context", but can remove if people are against it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't feel strongly.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Revised this section to make it clear that it's a general note on dynamic queries rather than specific to SQL queries.


First, carefully consider whether you really want to fully parameterize the column name. For example, users may choose a column that isn't indexed, making the query run extremely slowly and overload your database. Except for truly dynamic scenarios, it's usually better to have two SQL queries for two column names, rather than using parameterization to collapse them into a single SQL query.

If you've decided you do want to use parameterization, you'll have to use <xref:Microsoft.EntityFrameworkCore.RelationalQueryableExtensions.FromSqlRaw%2A>, which allows interpolating variable data directly into the SQL string, instead of using a database parameter:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is true. Using Where(e => EF.Property<object>(thePropertyName) == x) is a better way of doing this.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh sure, the idea was "if you've decided to use parameterization and have to do SQL as opposed to LINQ" (somewhat implied in the page context). I'll amend to make sure this is clearer.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But people might think that the only way to do this is with raw SQL.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tweaked the wording to make it clear that we're discussing dynamic construction only in the SQL context.

Copy link
Member

@ajcvickers ajcvickers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good stuff. Just make sure not to break links to existing docs.

@roji
Copy link
Member Author

roji commented Sep 20, 2022

/azp run

@azure-pipelines
Copy link

No pipelines are associated with this pull request.

* Rename "raw SQL" to "SQL queries"
* Switch to showing the new `FromSql` by default.
* Clearly document FromSqlRaw as appropriate only for dynamic SQL scenarios.
* Document SqlQuery
* Document ExecuteSql

Closes dotnet#3971
Closes dotnet#4041
Closes dotnet#1787
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants